Handle benchmark MXR dump as successful compile#5021
Closed
ikalinic wants to merge 2 commits into
Closed
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5021 +/- ##
===========================================
- Coverage 92.71% 92.71% -0.01%
===========================================
Files 596 596
Lines 31733 31736 +3
===========================================
Hits 29421 29421
- Misses 2312 2315 +3
🚀 New features to boost your workflow:
|
Regressions detected 🔴 |
|
pfultz2
requested changes
Jun 30, 2026
| * @brief Throw an exception with context information | ||
| */ | ||
| #define MIGRAPHX_THROW(...) throw migraphx::make_exception(MIGRAPHX_MAKE_SOURCE_CTX(), __VA_ARGS__) | ||
| #define MIGRAPHX_THROW_BENCHMARK_MXR_DUMPED(...) \ |
Collaborator
There was a problem hiding this comment.
This shouldn't be added to the errors. This a gpu-specific error.
| { | ||
| m.at(cmd)(ap, {args.begin() + 1, args.end()}); | ||
| } | ||
| catch(const migraphx::benchmark_mxr_dumped& e) |
Collaborator
There was a problem hiding this comment.
I dont really like catching exceptions in the driver as it reduces stopping at the exception in the debugger.
This also gpu-specific and the driver is for all targets.
Collaborator
There was a problem hiding this comment.
Stop reading my mind. I think this would be fine though if we caught a specific execption this is trying to block out vs a catch-all here?
| if(dump_mxr and is_root) | ||
| { | ||
| MIGRAPHX_THROW( | ||
| MIGRAPHX_THROW_BENCHMARK_MXR_DUMPED( |
Collaborator
There was a problem hiding this comment.
Instead of throwing an exception we can just print a message with log::info and then exit(0).
| { | ||
| f(); | ||
| } | ||
| catch(const benchmark_mxr_dumped& e) |
Collaborator
There was a problem hiding this comment.
The pass_manager shouldn't be catching gpu-specific exceptions.
TedThemistokleous
requested changes
Jun 30, 2026
TedThemistokleous
left a comment
TedThemistokleous
left a comment
Collaborator
There was a problem hiding this comment.
Remove the catch all - either catch the specific execption in the driver you're trying to avoid or make a clear log/warning and decide if you need to exit or can move forward
| { | ||
| m.at(cmd)(ap, {args.begin() + 1, args.end()}); | ||
| } | ||
| catch(const migraphx::benchmark_mxr_dumped& e) |
Collaborator
There was a problem hiding this comment.
Stop reading my mind. I think this would be fine though if we caught a specific execption this is trying to block out vs a catch-all here?
Member
Author
|
Closing in favor of #5025 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Avoid error logs and nonzero driver execution for the expected
migraphx-driver compilepath that dumps benchmark MXR files viaMIGRAPHX_GPU_DUMP_BENCHMARK_MXR.Technical Details
Added a dedicated
migraphx::benchmark_mxr_dumpedexception and throw macro, used it for the benchmark MXR dump completion path, logged it as info in the pass manager, and swallowed it at driver command dispatch so the dump-only mode completes successfully.Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable